Skip to content

feat: introduce CaveatType enum and normalize caveat type handling in builders#170

Closed
mj-kiwi wants to merge 18 commits intomainfrom
feat/caveat-type-enum
Closed

feat: introduce CaveatType enum and normalize caveat type handling in builders#170
mj-kiwi wants to merge 18 commits intomainfrom
feat/caveat-type-enum

Conversation

@mj-kiwi
Copy link
Contributor

@mj-kiwi mj-kiwi commented Feb 24, 2026

📝 Description

Apply the ScopeType enum pattern from PR #133 to caveat type parameters, allowing both enum references and string literals when defining caveats.

🔄 What Changed?

  • Added CaveatType enum to constants.ts with all 26 caveat enforcer function names
  • Created ConvertCaveatConfigsToInputs generic type to support flexible caveat type specification (enum or string)
  • Updated CaveatConfiguration type to use the flexible type conversion pattern
  • Enhanced CaveatBuilder.addCaveat() to normalize and accept both enum and string caveat types
  • Updated resolveCaveats() function to handle flexible caveat type normalization
  • Exported new types (CaveatType, CoreCaveatConfiguration, ConvertCaveatConfigsToInputs) in public API

🚀 Why?

  • Provides consistency with the ScopeType pattern established in PR chore: scope types #133
  • Improves API flexibility by allowing users to pass caveat types as either enum references or string literals
  • Maintains backward compatibility with existing string-based caveat type usage
  • Better type safety with enum references while preserving flexibility of string literals

🧪 How to Test?

  • Manual testing steps:
    1. Use CaveatType.AllowedMethods enum reference in caveatBuilder.addCaveat()
    2. Verify it works the same as string literal 'allowedMethods'
    3. Test both patterns in createDelegation caveats array
    4. Verify type completions show both patterns in IDE
  • Automated tests added/updated - Existing test suite passes
  • All existing tests pass ✓

⚠️ Breaking Changes

  • No breaking changes - This is fully backward compatible

📋 Checklist

  • Code follows the project's coding standards
  • Self-review completed
  • Documentation updated (if needed)
  • Tests added/updated - All existing tests pass
  • All CI checks pass ✓

🔗 Related Issues

Related to #145
Related to PR #133 (ScopeType enum pattern)
Related to PR #137 (enum flexibility pattern)

📚 Additional Notes

This implementation follows the same flexible type pattern established in PR #133 for ScopeType. Both enum references and string literals are supported for maximum flexibility:

// Using enum reference
caveatBuilder.addCaveat(CaveatType.AllowedMethods, { selectors: [...] })

// Using string literal (existing pattern)
caveatBuilder.addCaveat('allowedMethods', { selectors: [...] })

// In createDelegation caveats array
createDelegation({
  from: address,
  to: delegate,
  scope: scopeConfig,
  caveats: [
    { type: CaveatType.ValueLte, maxValue: 1000n },
    { type: 'erc20TransferAmount', tokenAddress: token, maxAmount: 100n }
  ]
})

Note

Medium Risk
Medium risk because it changes the public caveat-construction API surface (new CaveatType export, widened addCaveat/resolveCaveats typing, and updated error messaging), which could impact downstream TypeScript inference and caveat resolution if any mapping is incorrect.

Overview
Adds a new CaveatType enum (plus CaveatTypeParam) and reworks core caveat builder typing so addCaveat() and CaveatConfiguration accept either CaveatType.* or the existing string literal caveat names while preserving per-caveat config type checking.

Updates all core caveat builder modules and scope helpers to use CaveatType constants internally, exports CaveatType via the public kit + utils, and adjusts resolveCaveats() to accept enum/string type values while improving errors to include the failing caveat index.

Refreshes e2e and unit tests to use CaveatType, and adds new coverage proving enum and string forms behave identically (including explicit string-literal cases in allowedCalldata tests).

Written by Cursor Bugbot for commit 0c71b08. This will update automatically on new commits. Configure here.

@mj-kiwi mj-kiwi marked this pull request as ready for review February 24, 2026 06:34
@mj-kiwi mj-kiwi requested a review from a team as a code owner February 24, 2026 06:34
Copy link
Collaborator

@jeffsmale90 jeffsmale90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks super nice!

Would it make sense to add test coverage? I think for the most part behaviour hasn't changed (as the enum is a string anyways), but maybe it would be nice to have tests showing how to pass an enum in each of the cases.

I hope typescript errors in the tests would cause the tests to fail, so we can at least assert the type resolutions work as expected.

…ad of returning a value, allowing TypeScript to narrow the type and eliminating the need for variable reassignment

- Remove unnecessary 'as string' type assertion in resolveCaveats.ts - the type is already correctly inferred
- Move CaveatType enum from constants.ts to coreCaveatBuilder.ts to better organize the caveat-specific type definition
- Remove ConvertCaveatConfigsToInputs and CoreCaveatConfiguration from public exports - only expose CaveatType and essential builder types
- Update imports across the codebase to reflect the new locations
@mj-kiwi mj-kiwi marked this pull request as draft February 26, 2026 08:51
…lders

- Updated caveat builders to use the CaveatType enum instead of string literals for better type safety and maintainability.
- Removed the validateCaveatType function from utils as it is no longer necessary.
- Adjusted tests to reflect the changes in caveat type usage.
@mj-kiwi mj-kiwi marked this pull request as ready for review March 1, 2026 19:55
@mj-kiwi mj-kiwi requested a review from jeffsmale90 March 1, 2026 19:55
Copy link
Collaborator

@jeffsmale90 jeffsmale90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really good! It's definitely a struggle when changes touch every caveat type!

A couple minor comments - mostly I think we need to be able to:

caveatBuilder.add(CaveatType.AllowedCaveats, { ... }); // works 
caveatBuilder.add("allowedCaveats", { ...}); // doesn't work

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

…l support and update tests for compile-time validation
@mj-kiwi mj-kiwi requested a review from jeffsmale90 March 4, 2026 03:21
Copy link
Collaborator

@jeffsmale90 jeffsmale90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think generally this looks great.

There's something about the generics of the CaveatBuilder that's not quite sitting right. I've left a couple minor comments on the topic, but I'm going to have to spend a minute tomorrow getting my head around it properly. Maybe we can chat in the morning.

Comment on lines +149 to +154
export type CoreCaveatBuilder = CaveatBuilder<CoreCaveatMap> & {
addCaveat<TKey extends CaveatTypeParam>(
name: TKey,
config: CoreCaveatParamToConfig[TKey],
): CoreCaveatBuilder;
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to extend the CaveatBuilder type here with the shadowing addCaveat function? Does that indicate a deficiency with the CaveatBuilder type that might bite users who consume the CaveatBuilder directly (not the CoreCaveatBuilder)?

Comment on lines +92 to +94
* Supports both enum and string literal when the builder includes that enforcer, e.g.:
* - caveatBuilder.addCaveat(CaveatType.AllowedMethods, { ... })
* - caveatBuilder.addCaveat('allowedMethods', { ... })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this true of the CaveatBuilder itself? Or is this only true if TCaveatBuilderMap specifies keys that are a union of the enum and string values?

@mj-kiwi
Copy link
Contributor Author

mj-kiwi commented Mar 5, 2026

A better solution to add the CaveatType enum #179

@mj-kiwi mj-kiwi closed this Mar 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants